-
-
Notifications
You must be signed in to change notification settings - Fork 384
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: New Dark Scheme Part 2 #3317
Conversation
Hi @alifafaruk ! Thank you for the PR! The dark mode looks very good, but instead of adding a site specific toggle, I think it would make much more sense to just use the CSS prefers-color-scheme, as @teolemon suggested in the bug #2407 That way we can avoid the 2 javascript scripts that will block the page rendering for all users until they load, and we can have a pure CSS solution. Also it would be great to put the CSS styles inside the SCSS ( in the /scss/ directory) so that they get processed along with the rest of the CSS styles. cc @hangy |
Here's the Open Food Facts logo in dark mode: https://github.com/openfoodfacts/openfoodfacts-server/blob/3d1a30516a663d1dedad0733fac66d9d93a249c8/html/images/misc/openfoodfacts-logo-no-tagline-dark-mode.svg |
html/js/darkMode.js
Outdated
@@ -0,0 +1,30 @@ | |||
var stylesheet = document.styleSheets[3]; // dark-mode stylesheet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line of code is unrealible because order of stylesheets might be different on different pages and changed by newer code changes. If you want to use document.styleSheets
, at least use title
attribute, like shown in first example here
html/js/preDarkMode.js
Outdated
// Before page renders, check if page is in darkmode. | ||
// Otherwise, it will load regular styles before dark style. | ||
let dark = localStorage.getItem('darkMode'); | ||
var sheet = document.styleSheets[3]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same issue as in darkMode.js
It seems like |
This comment has been minimized.
This comment has been minimized.
Also, it seems like it might be possible to share "dark mode" setting across different domains but this might be complex to implement. I think that would require changes to perl code, docker config and nginx/apache config on production site. I don't have perl expertise to give more detailed advice. |
It sounds like a good idea to default to the dark mode, if someone has selected it, though. This guide explains that the media query can also be done from JS.
Changing some JavaScript should suffice, if I understand the blog post correctly. However, I wonder if we need |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add the new JS/CSS files to gulpfile.js
and then include them from the dist/js
/dist/css
directory, so that we can load the minified versions?
lib/ProductOpener/Display.pm
Outdated
@@ -6090,7 +6090,13 @@ $options{favicons} | |||
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/select2/4.0.10/css/select2.min.css" integrity="sha256-FdatTf20PQr/rWg+cAKfl6j4/IY3oohFAJ7gVC3M34E=" crossorigin="anonymous"> | |||
<link rel="search" href="$formatted_subdomain/cgi/opensearch.pl" type="application/opensearchdescription+xml" title="$Lang{site_name}{$lang}"> | |||
$header | |||
<!-- Dark Mode --> | |||
<link rel="stylesheet" href="$static_subdomain/css/darkMode.css"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This basically makes dark mode the default - Might introduce unwanted behaviour if JavaScript is disabled. cc @stephanegigandet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dark mode should not be the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test in all browsers but in Chromium adding disabled="true"
to link
disables stylesheet. I think this should work in other browsers as well. JS code of this PR is based on disabled
property and MDN says it's supported everywhere except IE (support is unknown)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test in all browsers but in Chromium adding
disabled="true"
tolink
disables stylesheet. I think this should work in other browsers as well. JS code of this PR is based ondisabled
property and MDN says it's supported everywhere except IE (support is unknown)
Right, but there are some extensions that completely disable Javascript execution. In that case dark mode would be the default. Consider adding disabled="true"
here and changing it in JS.
I don't know either. Is there some research on dark mode users? Do they turn it on or off depending on ambient light? Does it make sense to turn it on and off depending on the web sites users visit? I was under the impression that it was more of a "global" preference that is valid accross sites and times: you either want dark mode all the time, or you don't. So you set it once in the OS preference, and then hopefully web sites have styles that match your preference. |
The thing is, when I google "Chrome dark mode", I see this article which implies that dark mode is supported only in Win 10+ and MacOS 10.14+. As Ubuntu 18.04 user, I don't think I can enable it. So for various Linux users toggle might be useful. Toggle can be combined with |
I agree. Other sites (eg. stackoverflow, youtube, etc.) are using a site-specific switch and eventually we could add the "system settings" one. |
Hi all! Thank you for taking the time to look into the pull request. My group (@Jen-Lopez and @lucasortizny) and I are new to web development and are unfamiliar with the mechanics of how to implement a prefers-color-scheme. We will be fixing the CSS to fix the bugs. |
Hi Everyone, just wondering if there is still some work being done / to do on this PR? |
poke @alifafaruk @VaiTon |
Cannot push to branch :) |
Hi, we fixed all the style changes requested for dark mode. We also fixed bugs that were present. However, we don't know how to implement "prefers-color-scheme," so the method used to implement dark mode is still a toggle. If someone could help us change it to a "prefers-color-scheme" method, that would be great! |
lib/ProductOpener/Display.pm
Outdated
@@ -6090,7 +6090,13 @@ $options{favicons} | |||
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/select2/4.0.10/css/select2.min.css" integrity="sha256-FdatTf20PQr/rWg+cAKfl6j4/IY3oohFAJ7gVC3M34E=" crossorigin="anonymous"> | |||
<link rel="search" href="$formatted_subdomain/cgi/opensearch.pl" type="application/opensearchdescription+xml" title="$Lang{site_name}{$lang}"> | |||
$header | |||
<!-- Dark Mode --> | |||
<link rel="stylesheet" href="$static_subdomain/css/darkMode.css"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add disabled="true"
to link
tag
Good news, the dark mode compatible APK logo is merged, and available in prod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some parts are hard to read
This on the left, when user is logged in:
This is the bottom of page:
This light block (can be found at http://world.productopener.localhost/category/dairies):
Also, edit page (i.e. http://world.productopener.localhost/cgi/product.pl?type=edit&code=3908801793077) has a lot of light-colored blocks:
Everything else looks good to me
I fixed the "Edit a Product" Page and went throughout the site to fix more css bugs. Since the new Android image is available in production and is dark mode aware, this issue should be resolved. However, I was unable to fix the floating WebLinks block in "dairies" since there is no id/class associated with it to change the styling. What page should I look at to add an ID to the element? Also, if there are any changes to be made or any suggestions, please let me know. |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
This PR has merge conflicts. |
TODO items
|
We just deployed a new design, but without dark mode so far. |
We deployed a redesign of the https://world.openfoodfacts.org website, so sadly we won't be able to deploy this PR. |
Dark mode support
fixes #2407
@lucasortizny @Jen-Lopez and I worked on the dark mode feature for the website. We implemented a toggle button, where the user can turn dark mode on/off. We did a pull request earlier, but it had multiple merge conflicts that we weren't able to fix. So we closed that. We fixed the issues on our end and now it works!